-
-
Notifications
You must be signed in to change notification settings - Fork 9.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Storyshots: framework option should override package dependency check #1482
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1482 +/- ##
==========================================
- Coverage 14.6% 14.54% -0.07%
==========================================
Files 202 202
Lines 4655 4649 -6
Branches 508 515 +7
==========================================
- Hits 680 676 -4
+ Misses 3533 3522 -11
- Partials 442 451 +9
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't tested, but I think this is a good idea. What about you @tmeasday
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM (but didn't test)!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense to me apart from the package.json change.
@@ -24,6 +24,7 @@ | |||
"@storybook/channels": "file:../../lib/channels", | |||
"@storybook/channel-postmessage": "file:../../lib/channel-postmessage", | |||
"@storybook/react-native": "file:../../app/react-native", | |||
"@storybook/react": "file:../../app/react", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you just adding this line to prove that the fix works, or does it make sense to have it for the RNV app?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to prove it works
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok can we drop it then? Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds like this will be super easy to clear up and merge?
Done, Thanks @ktj ! |
Issue:
elderfo/react-native-storybook-loader#26
What I did
In storyshots addon I modified the framework check to make sure framework option overrides dependency check.
How to test
Reproduction project: https://github.com/ktj/storyshots-rp
-> tests fail
remove line 8 form package json -> tests work
I've added 3.1.9 example with latest react-native:
https://github.com/ktj/storyshots-rp/tree/319
I added storyshots with this fix as dependency.